Skip to content

fix(swift-sdk): act on swept transactions in the SwiftData store - #4589

Merged
lklimek merged 11 commits into
v4.2-devfrom
split/4406-5-swift
Sep 8, 2026
Merged

fix(swift-sdk): act on swept transactions in the SwiftData store#4589
lklimek merged 11 commits into
v4.2-devfrom
split/4406-5-swift

Conversation

@romchornyi

@romchornyi romchornyi commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Stacked on #4560. Review only this PR's own diff; its base is split/4406-3-producer.
Fourth of the five PRs #4406 was split into: seam → storage → producer → Swift → Kotlin.

Issue being fixed or feature implemented

Until this lands, the Swift host publishes the legacy struct_size, the negotiated sweeps slot reads None, CORE_SWEEP_REMOVAL is withheld, and Rust fail-closes: an iOS wallet freezes its sync watermark on the first sweep it meets rather than diverging. Funds-safe, but a user-visible stall — this is the PR that ends it.

What was done?

The store

SwiftData rows can be shared across wallets, so a sweep marks rather than deletes: isGloballySwept excludes the row and its outputs from every restore and enumeration path, and the physical delete is left to housekeeping once every wallet's scoped cleanup has landed. A tombstone must likewise outlive its loser — detach it and the consumed coin reads unspent again.

Held inputs become pending-input tombstones carrying the winner and, when it was mined, its height. A chained sweep repoints an earlier tombstone at the new winner rather than stacking a second hold. The release pass is outpoint-keyed, the drain gives tombstones precedence over ordinary observations, and isSpent stays monotonic against them: a hold the sweep proved consumed is never downgraded by a later record — not even the winner's own, which can arrive IS-locked, a context below in-block.

autosaveEnabled goes off on the round context. Sweeps travel in their own callback, so a round now spans two calls, and an autosave landing between them would make the watermark and the additive rows durable while the removal is still unstaged — with rollback() unable to take back a save that already happened. The handler attests ATOMIC_CHANGESETS and Rust relies on that to trust the split transport, so the guarantee has to be real.

Schema V4, and the freeze it required

The four models that gain a column (PersistentTransaction, PersistentTxo, PersistentPendingInput, PersistentWallet) were still referenced live by DashSchemaV1/V2/V3. Adding a property to a live model mutates those released versions' checksums in place, so a store written by a shipped binary matches no registered schema and fails to open with Cocoa 134504 instead of migrating — exactly what DashSchemaFrozenModels.swift was introduced to prevent, and its instruction is to freeze the model you change.

Freezing those four alone is not possible: a frozen model declares its relationships against frozen counterparts (an inverse: key path is typed on the destination model), and following relationships in both directions closes over 24 of the 35 models — a schema holds one type per entity name, so the component travels together. All 24 are frozen at their V3 shape, shared by V1/V2/V3, none of which changed any of them. The eleven models outside the component remain live-referenced and still carry the latent defect, unchanged by this PR.

DashSchemaV4 then registers the live models with a lightweight V3→V4 stage: every new column is additive with a default or optional, so existing rows migrate as not-swept, unsuperseded, ordinary unstamped claims, and a wallet with no chainlock boundary yet.

Merge with #4356

#4356 landed first and rewrote the same three regions. Its reconcileSpendObservation stays the single spend verdict, extended with one sweep term — a stamped hold outranks any observation — and its oldest-first pending-row reconciliation stays, under a tombstone-precedence branch.

One correction the merge forced: the "never displace confirmed evidence" rule refused the link when isSpent was true with no spender linked, which is precisely the sweep-hold shape, so the winner's own record could never supply the attribution the hold lacked. With no link there is nothing to displace, so it is adopted.

One gap neither PR covered is closed here: buildUnresolvedAssetLockTxRecordBuffer skips globally-swept rows, so the double-spend screen can never be handed a swept loser as the settled spender of a lock's input.

Also carried

The ChangesetRoundIndex per-round fetch cache — the reviewed-but-untested fix for the quadratic SwiftData fetch that put ~99% of CPU on the serial queue. Sweep paths deliberately opt out of it, since they key on mutable columns the index cannot answer stale. Named explicitly because it is the one piece here without dedicated tests.

How Has This Been Tested?

xcodebuild test -scheme SwiftDashSDK -destination 'platform=iOS Simulator,name=iPhone 17'437 tests, and swift build clean under the package's -warnings-as-errors.

  • SweptTransactionPersistTests (38): shared losers, detached tombstones with a missing winner row, chained tombstones, cross-round reinstatement, released-pending deadlock, co-swept twins, the throwing-lookup round failure, and the winner's late record against a stamped hold.
  • DashModelMigrationTests: gains testV3StoreMigratesToV4AndBackfillsTheSweepColumns, and its V1/V2 cases now write and read through the frozen types.
  • InvitationPersistenceTests tracks the new capability mask.

The only failures on this machine are two KeychainSignerAdditionalSigningKeysTests cases, which fail identically on an unmodified checkout — the bare xcodebuild run has no writable keychain, which run_tests.sh provides in CI.

Breaking Changes

None at the API surface. Schema V4 is a lightweight migration; the freeze exists specifically so V1/V2/V3 stores keep opening.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • New Features

    • Added support for tracking swept transactions, superseded outputs, and pending-input tombstones.
    • Added ChainLock height tracking to support safer finality-based cleanup.
    • Added capability indicators for sweep removal and DashPay payment persistence.
  • Bug Fixes

    • Improved spent-state resolution for swept and superseded transactions.
  • Migration

    • Added a lightweight migration to the latest persistence schema while preserving existing wallet data.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Warning

Review limit reached

Next included review available in 24 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c9e8710e-88bc-406d-89a1-13bb2ac4b1fb

📥 Commits

Reviewing files that changed from the base of the PR and between 27c7c08 and 7ea9be2.

📒 Files selected for processing (26)
  • packages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/11.json
  • packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabaseMigrationTest.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/DocumentDao.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TxoDao.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/WalletDao.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/PendingInputEntity.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/TxoEntity.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/WalletEntity.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabaseTest.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt
  • packages/rs-unified-sdk-jni/src/persistence.rs
  • packages/rs-unified-sdk-jni/src/wallet_manager.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashSchemaFrozenModels.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPendingInput.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTxo.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashModelMigrationTests.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/SweptTransactionPersistTests.swift

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: df7adecf-16b2-45cd-a841-51b0f9bdff76

📥 Commits

Reviewing files that changed from the base of the PR and between 66a7c74 and 27c7c08.

📒 Files selected for processing (11)
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashSchemaFrozenModels.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPendingInput.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTxo.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentWallet.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashModelMigrationTests.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/InvitationPersistenceTests.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/SweptTransactionPersistTests.swift

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The Swift SDK promotes the live persistence schema to V4, adds sweep and ChainLock fields, preserves earlier model shapes, registers lightweight migration, exposes new persistence capabilities, and expands migration and persistence tests.

Changes

Swift persistence schema and sweep state

Layer / File(s) Summary
Schema versioning and migration
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swift
Frozen model lists preserve V1 and V3 shapes. DashSchemaV4 registers the current models and adds a lightweight V3-to-V4 migration.
Sweep and finality persistence state
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPendingInput.swift, packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift, packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTxo.swift, packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentWallet.swift
The models store sweep tombstones, winner heights, global sweep status, TXO supersession, and the last applied ChainLock height.
Capability wiring and migration validation
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift, packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashModelMigrationTests.swift, packages/swift-sdk/SwiftTests/SwiftDashSDKTests/InvitationPersistenceTests.swift
The SDK exposes sweep and DashPay capability flags. Tests validate frozen schema models, V3-to-V4 migration, capability reporting, and invitation persistence results.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 27c7c

The schema migration, persistence-model updates, and capability coverage present no supported merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: SwiftData now processes swept transactions in the Swift SDK.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch split/4406-5-swift

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

🕓 Queued for automated review — 65th in line, estimated start in ~91 h (commit 7ea9be2)
Estimated review time once started: ~2.8 h (two-phase automated review; median of recent runs).

  • Request priority review — tick this box and the review moves to the front of the queue.

Bumps the rust-dashcore pin to dev and projects the `TransactionsSwept`
event the bump brings with it. The two halves are one commit by
construction: `WalletEvent` is not `#[non_exhaustive]` and platform has
four exhaustive matches over it, so new-pin code cannot compile without
the arms — and arms that did nothing would be worse than none, because
upstream's removal is unconditional. The wallet drops the losing rows in
memory; a store that keeps them replays them at the next load and
re-creates the phantom balance the upstream fix exists to kill.

The projection is one `SweepBatch` per event, and a sweep-only round is
counted in `is_empty_no_records` so a round carrying nothing but a sweep
still reaches the persister.

The gate is what makes every intermediate host state safe. A backend
that has not attested `CORE_SWEEP_REMOVAL` is not known to have applied
the round's subtractive half, so its watermark is stripped BEFORE the
store and the wallet faults exactly as it would on a rejection —
reporting the height durable first and faulting after cannot retract a
height a legacy backend already committed. Such a host freezes its sync
watermark on the first sweep it meets instead of diverging: fail-closed,
funds-safe, and unfrozen the moment its persister ships.

A record arriving after a sweep of the same txid retracts that txid from
the folded sweep, since persisters write records before replaying sweeps
and would otherwise delete a row the wallet has brought back. The
asset-lock half mirrors it: a sweep removes the tracked entry its
funding transaction created, and `AssetLockChangeSet::merge` now cancels
a folded tombstone against a reinstating upsert (and vice versa), so no
store ever sees an upsert/tombstone pair for one outpoint whose outcome
depends on which it applies first.

The pin also carries rust-dashcore#981, which collapses BIP-39 parsing
onto one auto-detecting path. Platform's four hand-rolled
"try every wordlist" helpers are now that function, and the call sites
drop their `Language` argument. It is unrelated to sweeps and rides here
only because the sweep chain and the payload-finalization seam this
branch's base already depends on both sit above it on dev.

`spend_observer`'s two projections gain sweep arms that report no
observed spend: a sweep's released outpoints are coins that came back
free, and the inputs it kept spent are precisely the ones it does not
name, so the held set cannot be derived from the event at all.
…ouched

`cargo fmt --check --all` is a CI gate and the collapsed
`Mnemonic::from_phrase` calls left two of them wrapped.
Review nits, all documentation.

`parse_mnemonic_any_language`'s doc still said `key_wallet::Mnemonic`
"only exposes language-tagged constructors" and that callers "must walk
the language list themselves" — precisely what rust-dashcore#981
removed, and it contradicted the inline comment three lines below. The
wrapper is kept: 20 call sites narrow upstream's error to the
`&'static str` they report, and that narrowing is now what the doc says
it does.

The sweep gate's recovery note read as if a capable backend might appear
mid-session. It cannot: the persister does not change under a running
adapter, so a host without the slot stays frozen until it ships one and
relaunches. Freezing is the point.

`last_processed_height` is now documented as deliberately NOT stripped
beside `synced_height`, matching the #4069 guard: `synced_height` is the
durable "scanned AND persisted" claim that must not outrun an unapplied
removal, while `last_processed_height` is the adapter's own progress
marker whose retention makes nothing safer.

And the asset-lock test's `DASHPAY_PAYMENTS` attestation no longer
describes an overlay this PR writes — nothing here stages
`dashpay_payments_overlay`; the bit is declared so the fixture still
describes a fully capable backend once #4442 lands.

Not taken: de-indenting the vestigial block in `commit_wallet`. It spans
152 lines, so removing it would bury the reviewable diff under a
whitespace-only change and force another rebase of the four PRs stacked
above this one.
…reason

CI lints these crates with `-D warnings`, so clippy's seven-argument
threshold is an error, and the #4370 merge gave `commit_wallet` an
eighth: the `settled` set the panic arm in `run_wallet_event_adapter`
reads back to decide which wallets have an unknown outcome. Every
parameter is a distinct piece of drain state this function reads and
writes, and the borrow split is what keeps them separately mutable —
bundling them would rename the same eight.
The SwiftData mirror of the storage contract, complicated by two things
SQLite does not have: rows shared across wallets, and a round that now
spans two callbacks.

Shared rows are why a sweep marks rather than deletes. A transaction row
can belong to several wallets, so the first wallet's callback cannot
remove it — it sets `isGloballySwept`, which excludes the row and its
outputs from every restore and enumeration path, and the physical delete
is left to housekeeping once every wallet's scoped cleanup has landed. A
tombstone must likewise outlive its loser: detach it and the consumed
coin reads unspent again.

Held inputs become pending-input tombstones carrying the winner and,
when it was mined, its height; a chained sweep repoints an earlier
tombstone at the new winner rather than stacking a second hold. The
release pass is outpoint-keyed, the drain gives tombstones precedence
over ordinary observations, and `isSpent` stays monotonic against them:
a hold the sweep proved consumed is never downgraded by a later record —
not even the winner's own, which can arrive IS-locked, a context below
in-block.

`autosaveEnabled` goes off on the round context. Sweeps travel in their
own callback, so the round spans two calls, and an autosave landing
between them would make the watermark and the additive rows durable
while the removal is still unstaged — with `rollback()` unable to take
back a save that already happened. The handler attests
`ATOMIC_CHANGESETS`, and Rust now relies on that to trust the split
transport, so the guarantee has to be real. The handler declares
`CORE_SWEEP_REMOVAL` and `DASHPAY_PAYMENTS`; before this commit it
published the legacy `struct_size`, the negotiated slot read `None`, and
Rust fail-closed.

The four models that gain a column — `PersistentTransaction`,
`PersistentTxo`, `PersistentPendingInput`, `PersistentWallet` — were
still referenced live by `DashSchemaV1/V2/V3`. Adding a property to a
live model mutates those released versions' checksums in place, so a
store written by a shipped binary would match no registered schema and
fail to open with Cocoa 134504 instead of migrating. That is exactly the
defect `DashSchemaFrozenModels.swift` was introduced to prevent, and its
instruction is to freeze the model you change.

Freezing those four alone is not possible: a frozen model declares its
relationships against frozen counterparts (an `inverse:` key path is
typed on the destination model), and following relationships in both
directions closes over 24 of the 35 models — one type per entity name is
all a schema can hold, so the component travels together. All 24 are
frozen here at their V3 shape, shared by V1, V2 and V3, none of which
changed any of them. The eleven models outside the component are still
live-referenced and still carry the latent defect, unchanged by this.

`DashSchemaV4` then registers the live models with a lightweight V3→V4
stage: every new column is additive with a default or optional, so
existing rows migrate as not-swept, unsuperseded, ordinary unstamped
claims, and a wallet with no chainlock boundary yet.

`reconcileSpendObservation` stays the single spend verdict, extended
with one sweep term — a stamped hold outranks any observation — and its
oldest-first pending-row reconciliation stays, under a
tombstone-precedence branch. One correction the merge forced: the "never
displace confirmed evidence" rule refused the link when `isSpent` was
true with NO spender linked, which is precisely the sweep-hold shape, so
the winner's own record could never supply the attribution the hold
lacked. With no link there is nothing to displace, so it is adopted.

One gap neither PR covered is closed here:
`buildUnresolvedAssetLockTxRecordBuffer` now skips globally-swept rows,
so the double-spend screen can never be handed a swept loser as the
settled spender of a lock's input.

Also carries the `ChangesetRoundIndex` per-round fetch cache — the
reviewed-but-untested fix for the quadratic SwiftData fetch that put
~99% of CPU on the serial queue. Sweep paths deliberately opt out of it,
since they key on mutable columns the index cannot answer stale.

Tests: `SweptTransactionPersistTests` (38) — shared losers, detached
tombstones with a missing winner row, chained tombstones, cross-round
reinstatement, released-pending deadlock, co-swept twins, the
throwing-lookup round failure, and the winner's late record against a
stamped hold. `DashModelMigrationTests` gains the V3→V4 stage and reads
V1/V2 rows through the frozen types. Full suite: 437 tests, the only
failures being two `KeychainSignerAdditionalSigningKeysTests` cases that
fail identically on an unmodified checkout (the test host cannot write
to the keychain).
@romchornyi

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@llbartekll llbartekll left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed this PR's own diff only (base split/4406-3-producer), against the four things it sets out to do: the sweep application path, autosaveEnabled = false, schema V4 + the freeze, and the round index.

The freeze checks out. I compared all 24 frozen copies against the live models on the base branch mechanically — property names, types, defaults, @Attribute, #Index and @Relationship markers all match; the only textual differences are the public modifiers, which are not schema inputs, plus the expected V1 PersistentAssetLock delta. componentFrozenModelTypes is positionally identical to allModelTypes (34 entries, assetLock in the same slot). I also checked txid byte order across the new sweep path (withUnsafeBytes on the FFI tuple) against the record path's hashData — both are raw bytes, so sweep txids match stored ones. That was the highest-risk part of the change and it is sound.

Two things I'd like addressed before this merges:

  1. Seven mangled lines from what looks like an automated edit — two of them in persistTrackedMasternodes, which this PR does not otherwise touch. Cosmetic, but they shouldn't land.
  2. settledSpenderLinkIsKept is never called, and the rule it documents is absent from the live path. Details inline — this is the only finding with correctness weight.

The rest (tombstone collector scan cost, the unbracketed payments path under autosave-off, two test gaps) is non-blocking — take or leave as follow-ups.

I did not run xcodebuild test on my side, so the 437-test claim is taken at face value.

…e per wallet

Review follow-ups on the SwiftData sweep writer, brought onto the same
doctrine as the SQLite store (#4559). Every rule below is a property of
the coin, not of the row's relationships.

The hold is keyed by outpoint. `applySweptTransaction` decodes the
loser's inputs from its stored bytes and settles each one by key; a
spender link is detached only if it points at the loser. Before, the
sweep walked the loser's `inputs` relationship, which a store-only fetch
had refreshed to its saved state — so a winner recorded in the same
round as the sweep of its loser had its freshly written link nil-ed, and
`walletFundedTransaction` never saw the winner again. The one link
writer, `adoptSpendObservation`, now registers the displaced spender in
the round index, so no keyed store-only lookup can refresh an object
carrying staged state.

The hold is global, the release is per wallet. The first callback that
sees a sweep holds every wallet's rows for the loser's inputs, then
deletes the loser's row; each wallet's own callback applies its release
set to its own rows. The loser's outputs are dead for everyone and its
inputs' holds are a txid fact, not a per-wallet one — only the release
set depends on which records a wallet holds. That removes
`isGloballySwept`, the deferred delete and every reader guard built to
hide a surviving swept row; a swept row no longer enumerates through
`involvedTransactions`, and a wallet whose round is rejected finds its
coin held rather than restorable.

Pending rows are per (outpoint, spendingTxid, walletId), so a second
wallet recording the same spend keeps its own claim row; the drain
prefers the tombstone tagged with the delivering wallet. A drained
tombstone stamps — `isSpent`, `supersededByTxid` — and never mints a
spender link or a vin index; the winner's own claim row beside it
supplies both. A release is vetoed by a stored network-final claim whose
bytes actually spend the outpoint (a stamp alone does not veto: under
the global hold it lands on every non-released input). The settled-link
guard is wired for real: `reconcileSpendObservation` takes the existing
spender's context, `isSpent` is monotonic on both channels, and a
stamped, unlinked coin the wallet re-delivers unspent follows the wallet
— refusing would lock a real coin out of every future restore after a
reorg of its winner.

The collector runs once per round, from `endChangeset`, after every
account slice and every sweep, on the boundary the round's own writes
left on the wallet row. Before, it ran in the header — before this
round's `utxos_added` — so a funding output arriving in the round that
completed the boundary found its tombstone already deleted and landed
unspent. The store query now selects tombstones only, with an index on
`[walletId, isSweptTombstone]` in the V4 stage.

Also: out-of-round save failures roll the context back and log when the
round index has to fall back; the seven `print` sites go through
`SDKLogger`; `hashData(_:)`, one tombstone-scan helper and one
wallet-lookup preamble replace the inline copies; the seven collapsed
newlines are restored; the V4 columns are documented under V4 and the
frozen-models header describes what is actually frozen; the migration
test asserts V3 and V4 name the same entity set.

Tests: `SweptTransactionPersistTests` 38 → 50, eighteen of them red on
the pre-fix handler; `swift test` 461 passed.
romchornyi pushed a commit that referenced this pull request Sep 8, 2026
…se per wallet

Review follow-ups on the Room sweep writer, brought onto the same
doctrine as the SQLite store (#4559) and the Swift port (#4589). Every
rule below is a property of the coin, not of a row's foreign key.

The hold is keyed by outpoint. The sweep decodes the loser's inputs from
its stored bytes (`StoredTransactionInputs`, key-wallet's
`transaction_decode`, txid verified, undecodable fails the round closed)
and holds each one by key: `isSpent = 1`, `supersededByTxid = winner`,
any non-loser spender link kept. Before, the hold was `UPDATE … WHERE
spendingTxid = loser`, so a winner whose own record landed in the same
round — the common path for the wallet's own double-spends — had already
taken the link, nothing matched, and the coin restored as spendable
after a restart until the winner mined.

The hold is global, the release is per wallet. The first callback that
sees a sweep holds every wallet's rows for the loser's inputs, then
deletes the loser's row (hold before delete, so the FK `SET NULL` and
cascade only clear links); each wallet's own callback applies its
release set to its own rows, by outpoint. That removes `isGloballySwept`
— column, migration, flag maintenance — `hasOtherWalletClaim` and its
probes, the deferred delete and every reader guard built to hide a
surviving swept row. A wallet whose round is rejected now finds its coin
held rather than restorable, which is what the class doc claimed.

Pending rows are per (outpoint, spendingTxid, walletId); the drain
prefers the delivering wallet's tombstone. A drained tombstone stamps
and never mints a spender link, so a later release can still free the
coin. A release is vetoed by a stored network-final claim — a linked
spender, or a stamp whose stored bytes actually spend the outpoint. One
`linkSpender` serves the record, `utxos_spent` and drain channels;
`isSpent` is monotonic on all of them; a stamped, unlinked coin the
wallet re-delivers unspent follows the wallet. The found-TXO branch
deletes only the arriving txid's pending rows when the existing link is
kept.

Batches are buffered per round and applied together, so the co-swept
set spans the round, and the collector runs once from `onChangesetEnd`
after every slice and sweep — before, it ran in the header, ahead of the
round's own `utxos_added`. The chainlock height is a narrow monotonic
`UPDATE`. Per-loser statement fan-out is replaced by chunked `IN (:chunk)`
forms and rowid-keyed pending writes.

Schema: one `MIGRATION_10_11` (four columns, two indexes on
`pending_inputs`), version 11, `11.json` regenerated by Room.

JNI: the sweep slot ships flat `[B` arrays with counts, one `supersededBy`,
and an explicit `(hasWinnerMinedHeight, winnerMinedHeight)` pair —
descriptor `([B[BI[B[BIZI)I`, pinned by a unit test. Heights cross the
boundary through a checked `u32 → Int` conversion that fails the round
closed rather than wrapping negative. The sweep slot is wired only when
the concrete bridge overrides the method.

Every ported KDoc cites its Swift source; the "unreachable on this
channel" clause is gone; the changeset → chainlock-height → sweeps order
is stated once.

Tests: handler class 133 → 150, `DashDatabaseTest` 10 → 12, the
behavioural cases red with the pre-fix behaviour re-introduced; 422
passed. `rs-unified-sdk-jni` 39 passed.
…del from V1

Two gaps from review. The settled-link guard's own case — a plain
in-block arrival against a spender that is only IS-locked — was covered
only through the mempool variant, which the pre-existing `isSpent`
branch would have refused anyway. The new case delivers the conflicting
record at context 2 against an unmined IS-locked spender, checks the
link stays and the coin stays spent through the following sweep's
release, and pins chainlock-over-IS-lock as the one takeover.

The V3 → V4 migration test wrote only a wallet and a pending row, so two
of the four widened models never crossed the stage; it now carries a
transaction and a spent coin through and asserts the stamp backfills to
nil. A new V1 → V4 case migrates a wallet, a transaction and a coin from
the oldest registered version, including the coin's relationship to its
funding transaction — the freeze pinned where it matters.
@romchornyi

Copy link
Copy Markdown
Contributor Author

@llbartekll thanks — both blockers and all four follow-ups are addressed, replies inline. Two commits:

  • f0fdcc1da7 is larger than the two blockers because the review pass that found the dead settledSpenderLinkIsKept also found the shape behind it: the hold was keyed by the spender link (a relationship a store-only fetch can refresh away), while upstream's release is keyed by outpoint. The commit moves the Swift store onto the same doctrine as the SQLite store in fix(platform-wallet-storage): durably apply swept transactions in the SQLite store #4559: the hold is computed from the loser's decoded inputs by outpoint and detaches only the loser's own link; the hold is global and the release per wallet, which deletes a swept row in the first callback and removes isGloballySwept and every reader guard built on it; a drained tombstone stamps without minting a link; the collector runs once, at the end of the round, after utxos_added. The commit message has the full list; SweptTransactionPersistTests went 38 → 50, eighteen of them red on the previous handler.
  • a93c4190a0 adds the two tests you asked for (in-block-vs-IS-lock; V1 → V4 with a wallet, a transaction and a coin) and widens the V3 → V4 case to every model V4 changes.

swift test: 462 tests, 0 failures, run locally against a mac slice built from this branch. The Kotlin port (#4590) got the same rework in 332d548758 and merges this branch.

llbartekll
llbartekll previously approved these changes Sep 8, 2026

@llbartekll llbartekll left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Re-reviewed f0fdcc1da7 + a93c4190a0 in full — this went well past fixing the seven points.

On my findings:

  1. Formatting — all seven restored, and persistTrackedMasternodes is out of the diff. Re-scanned the file for glued braces and run-on continuations: clean.
  2. settledSpenderLinkIsKept — now actually wired: reconcileSpendObservation takes the existing spender's context and consults it, and every channel (record pass, utxos_spent emit, pending drain) goes through one link writer in adoptSpendObservation. testAnInBlockArrivalDoesNotTakeTheLinkFromAnInstantSendLockedSpender pins exactly the case that fell through before, including the release veto in the following sweep, with the chainlocked takeover in the second half.
  3. Collector — once per round from endChangeset, gated on a round that actually moved a boundary half, and the scan now selects isSweptTombstone == true behind a new (walletId, isSweptTombstone) index instead of materialising every pending row. Worth more than the perf note: moving it after the slices caught a real funds bug (the boundary-completing round deleting a tombstone before the same round's utxos_added could drain it), now pinned by testAFundingOutputDeliveredInTheRoundThatCompletesTheBoundaryStillDrainsItsTombstone.
  4. Unbracketed payments path — checked against the FFI, and both consequences are now non-silent (rollback on a failed out-of-round save, persistence_round_index_disabled when a dirty context costs the round its index).
  5. Migration tests — testV1StoreWithWalletTransactionAndCoinMigratesToV4 covers what I asked for, and testV3AndV4NameTheSameEntitySet pins the entity-set invariant the redesign now depends on.

On the redesign itself (dropping isGloballySwept, keying the hold on the outpoint, holding globally and releasing per wallet): I read it end to end rather than treating it as a fixup, and it holds together — the hold now survives the row rather than depending on it, so the delete is unconditional and the cross-wallet ordering problem the flag existed to paper over is gone, along with the deadlock it needed the released-pending special case for. Keying off the loser's decoded inputs instead of row.inputs closes the case where the link had already moved to the winner. releaseIsVetoed reads as the right place for that check, and failing closed on a network-final claimant whose bytes don't decode is the correct direction to fail.

I re-ran the mechanical check on the new state: all 25 frozen copies still match the live models on the base branch exactly — attributes, optionality, defaults, @Attribute/#Index/@Relationship markers — with only the documented V2 asset-lock delta. Entity set unchanged from V3, so the schema story survives PersistentTransaction no longer being widened.

One thing I'd flag without blocking, since it is a deliberate semantic reversal from the first revision and your call as the author of the storage contract: isSpent is now monotonic on the observation channels, with utxos_added re-delivery as the single path down — the old reorg-demotion lowering (re-observing the linked spender at a lower context) is gone, and a stamped materialised hold now frees on re-delivery instead of refusing it. The reasoning in the property docs is sound (the wallet knows the coin, so BIP158 prevout matching re-discovers any network-final spender, and refusing would strand a real coin after a reorg of the winner), and both directions are pinned — testWalletReDeliveringAMaterialisedHeldCoinFreesIt and testWalletReDeliveringACoinLinkedToASettledSpenderKeepsItSpent. Just worth being deliberate that the safety of the whole thing now rests on the wallet re-emitting a coin it holds unspent.

Test count went 38 → 51 on the sweep suite. I still haven't run xcodebuild test locally, and I notice CI has no Swift job (Kotlin + the explorer-model check are what run here), so the suite result is on your word — nothing in this review depends on it.

Base automatically changed from split/4406-3-producer to v4.2-dev September 8, 2026 09:27
@lklimek
lklimek dismissed llbartekll’s stale review September 8, 2026 09:27

The base branch was changed.

@github-actions github-actions Bot added this to the v4.2.0 milestone Sep 8, 2026
romchornyi and others added 3 commits September 8, 2026 11:27
Co-authored-by: Roman <51091564+jeanpierreroma@users.noreply.github.com>
Brings in the squash-merged producer (#4560) this branch was stacked on,
plus #4584. Conflicts in changeset.rs and core_bridge.rs were this
branch's copies of the producer commits against their squash; this
branch never touched either file, so the base version was taken and the
tree outside packages/swift-sdk is identical to v4.2-dev.
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (5f58417) to head (7ea9be2).

Additional details and impacted files
@@             Coverage Diff              @@
##           v4.2-dev    #4589      +/-   ##
============================================
+ Coverage     85.70%   85.91%   +0.20%     
============================================
  Files          2764     2794      +30     
  Lines        367624   370651    +3027     
============================================
+ Hits         315076   318432    +3356     
+ Misses        52548    52219     -329     
Components Coverage Δ
dpp 86.58% <ø> (+0.60%) ⬆️
drive 84.97% <ø> (+0.23%) ⬆️
drive-abci 88.95% <ø> (+0.28%) ⬆️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.92% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 41.44% <ø> (+0.33%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

`cargo fmt --check` is a CI gate; the checked `u32 -> Int` conversion
added for the sweep and header slots was hand-written.
@lklimek
lklimek merged commit 8bd3e53 into v4.2-dev Sep 8, 2026
19 checks passed
@lklimek
lklimek deleted the split/4406-5-swift branch September 8, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants